feat: add X-Docker-Agent-Version and X-Docker-Desktop-Version headers to built-in tools#2795
Conversation
Introduce SetIdentity() to centralize HTTP identity headers (User-Agent, X-Docker-Agent-Version, X-Docker-Desktop-Version) used across built-in tools. Add pkg/desktop/version.go to fetch Docker Desktop version from the backend socket's /update endpoint once per process (memoized, 2s timeout).
Update api tool to use the new SetIdentity() helper, ensuring consistent identity header formatting across all built-in tools. Add test coverage for identity headers.
Update fetch tool to use SetIdentity(), including robots.txt requests. Remove redundant userAgent parameter from fetchRobots(). Add test coverage for identity headers.
Update openapi tool to use SetIdentity(), establishing unified identity header handling. Add test coverage for identity headers.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
One medium confirmed finding in the new code.
| CurrentVersion string `json:"currentVersion"` | ||
| } | ||
| _ = ClientBackend.Get(ctx, "/update", &info) | ||
| return info.CurrentVersion, nil |
There was a problem hiding this comment.
[MEDIUM] Empty Desktop version cached for full 5-minute TTL when Desktop is unavailable at first call
GetVersion always returns (info.CurrentVersion, nil) — even when the HTTP call to Desktop's /update endpoint fails (Desktop not running, socket not present, timeout, etc.). In that case info.CurrentVersion is the zero value "", and since go-memoize only caches results where error == nil, the empty string gets cached for the full 5-minute TTL.
This contradicts the PR description's claim that the lookup "recovers automatically if Desktop starts after docker-agent": if GetVersion is first called before Desktop is ready, the X-Docker-Desktop-Version header will be silently omitted for up to 5 minutes even after Desktop becomes available — rather than recovering on the next request.
Fix: return a non-nil error when the lookup fails so go-memoize does not cache the failure:
if err := ClientBackend.Get(ctx, "/update", &info); err != nil {
return "", err // not cached; next caller retries
}
return info.CurrentVersion, nilThis way a transient failure (Desktop not yet up) never poisons the cache.
| var info struct { | ||
| CurrentVersion string `json:"currentVersion"` | ||
| } | ||
| _ = ClientBackend.Get(ctx, "/update", &info) |
There was a problem hiding this comment.
Non-blocking: the _ = ClientBackend.Get(...) ignores the error and lets the memoizer cache the empty string for the full 5 minutes. That's intentional for the steady-state "Desktop not running" case (good — we don't want to hammer the socket), but it also means a transient failure (e.g. backend.sock momentarily unavailable while Desktop restarts, or a 2 s timeout under load) gets pinned for 5 minutes before we try again.
If we wanted to differentiate, returning a non-nil error from the closure on Get failures would let go-memoize re-attempt sooner (its TTL applies per cache entry, but errors aren't cached the same way — worth double-checking the library semantics). Not a blocker for this PR — the header is best-effort by design — but worth a comment in the code so the next reader knows the swallow is deliberate.
| // recovers automatically once Desktop comes up; | ||
| // - if Desktop is upgraded mid-session, the new version is picked up | ||
| // within at most one TTL. | ||
| var versionMemoizer = memoize.NewMemoizer(5*time.Minute, 10*time.Minute) |
There was a problem hiding this comment.
Question: what drove the 5 min TTL (and the 10 min cleanup interval)? The PR description mentions "recovers automatically … upgraded mid-session" which argues for some refresh, but 5 minutes is a fairly arbitrary middle ground — Desktop upgrades are rare (once a week), the lookup is cheap-ish (one local socket call with a 2 s ceiling), and the value almost never changes within a session.
A short rationale comment near the NewMemoizer call (e.g. "5 min keeps the steady-state cost ≤ 1 socket call per 5 min per process while still picking up Desktop upgrades within a few minutes") would save the next reader the same question. Happy to leave the value as-is.
| req.Header.Set("User-Agent", Header) | ||
| req.Header.Set(HeaderAgentVersion, version.Version) | ||
| if v := desktop.GetVersion(req.Context()); v != "" { | ||
| req.Header.Set(HeaderDesktopVersion, v) |
There was a problem hiding this comment.
Non-blocking (privacy/scope): SetIdentity is wired into every api / fetch / openapi outbound request, which means X-Docker-Desktop-Version is broadcast to every host the operator points a built-in tool at — not just Docker-owned backends. The PR description acknowledges this and defers an opt-out, which I think is fine, but two thoughts for the next iteration:
- Consider scoping the Desktop header to known Docker hosts (Hub, Hub gateway,
*.docker.com,*.docker.io) by default. The agent-version header is harmless to broadcast; the Desktop-version one is the genuinely new signal and the one most likely to surprise a security-minded operator. - If we keep it global, the README / docs should mention the new header explicitly so operators auditing outbound traffic aren't surprised. I don't see a docs update in this PR — happy to merge without one if a follow-up is tracked.
Neither blocks this PR.
|
|
||
| assert.Equal(t, Header, req.Header.Get("User-Agent")) | ||
| assert.Equal(t, version.Version, req.Header.Get(HeaderAgentVersion)) | ||
| } |
There was a problem hiding this comment.
Non-blocking: two behaviours that the PR description calls out as load-bearing don't have a unit test here:
- Desktop unreachable →
X-Docker-Desktop-Versionomitted, not set to empty string. Today this is implicit (because no Desktop socket is around in CI) — the existing tests inapi/fetch/openapiassert presence of the agent-version header but don't assert absence of the Desktop one. A test that injects a stub returning""and assertsreq.Header.Get(HeaderDesktopVersion) == ""and that the key is not present inreq.Headerwould pin the contract. - Operator-supplied override wins. The
setHeaderscallers apply caller headers afterSetIdentity, which is the documented precedence — but there's no test pinning that aUser-AgentorX-Docker-Agent-Versionin the operator config overrides the defaults.pkg/tools/builtin/fetch/fetch_test.gohas aTestFetch_Headers_OverrideDefaultsforUser-Agent; an analogous one forX-Docker-Agent-Version(and ideally one inapi/openapi) would catch a future refactor that flips the order.
Both are quick to add and would lock in the invariants the PR description leans on.
3 participants
What
Adds two HTTP identity headers to every outbound request made by the
api,fetch, andopenapibuilt-in tools, on top of the existingUser-Agent:User-AgentCagent/<version> (<goos>; <goarch>)X-Docker-Agent-Version<version>(e.g.dev,1.2.3)X-Docker-Desktop-Version4.74.0Operator-supplied headers in the agent config still win over these defaults —
SetIdentityis called first, then caller headers.Why
Backends that receive built-in-tool traffic (Docker Hub APIs, Hub gateway, ...) want to attribute requests to a specific docker-agent install and the Docker Desktop version it's running alongside, the same way other Docker traffic is already attributed.
User-Agentwas the only signal so far and is brittle to parse.How
pkg/useragent.SetIdentity(req)is the single source of truth for these headers. All three built-in tools route through it, including therobots.txtfetch in thefetchtool.pkg/desktop.GetVersion(ctx)readscurrentVersionfrom Docker Desktop'sbackend.sock/updateendpoint, with:memoize.Call[string]) — recovers automatically if Desktop starts after docker-agent or is upgraded mid-session, while still costing one socket call per tool request at most;""when Desktop isn't running, in which case the header is omitted.No new dependencies (
go-memoizeandgo-cachewere already vendored).Privacy / security note
X-Docker-Agent-Versionadds no information thatUser-Agentdidn't already broadcast.X-Docker-Desktop-Versionis genuinely new data sent to whichever host the operator configured. It's no more leaky thanUser-Agentis for<goos>; <goarch>, but if a privacy/opt-out mode is desired we can layer one on top later. Not adding one now keeps this PR focused.Tests
pkg/useragent: pins User-Agent +X-Docker-Agent-Versionon a fresh request.api,fetch,openapi: each existing header test now also asserts the two identity headers reach the test server.task lintis clean.Commits
feat: add identity headers for docker-agent and Docker Desktop trackingrefactor: use SetIdentity() in api toolrefactor: use SetIdentity() in fetch toolrefactor: use SetIdentity() in openapi toolfix(desktop): make Desktop version lookup TTL-based and context-independentrefactor: simplify Desktop version lookup and useragent docsrefactor(desktop): plumb context through GetVersion